protect invariance of OptionInt, OptionDouble (#1363)

* protect invariance of OptionInt, OptionDouble

we want to ensure the source string always corresponds to the
results of conversion.

* fix spelling in this mornings grammar fix!

* remove obsolete include

diff --git a/option.cc b/option.cc
index 5f9c88789ebc70551e44a1bffee5cc6474c5d679..8234bf0aa2813ef6c933cf7693fb42513c0910c9 100644 (file)
--- a/option.cc
+++ b/option.cc
@@ -52,3 +52,66 @@ double OptionString::toDouble(bool* ok, QString* end) const
 {
   return parse_double(value_, id_, ok, end);
 }
+
+void OptionInt::init(const QString& id, bool allow_trailing_data, int base)
+{
+  id_ = id;
+  base_ = base;
+  allow_trailing_data_ = allow_trailing_data;
+}
+
+void OptionInt::reset()
+{
+  value_ = QString();
+  result_ = 0;
+  end_ = QString();
+}
+
+void OptionInt::set(const QString& s)
+{
+  value_ = s;
+
+  // Fatal on conversion error.
+  QString* endp = allow_trailing_data_? &end_: nullptr;
+  constexpr bool* dieonerror = nullptr;
+  result_ = parse_integer(value_, id_, dieonerror, endp, base_);
+}
+
+int OptionInt::get_result(QString* end) const
+{
+  if (end != nullptr) {
+    *end = end_;
+  }
+  return result_;
+}
+
+void OptionDouble::init(const QString& id, bool allow_trailing_data, int /* base */)
+{
+  id_ = id;
+  allow_trailing_data_ = allow_trailing_data;
+}
+
+void OptionDouble::reset()
+{
+  value_ = QString();
+  result_ = 0.0;
+  end_ = QString();
+}
+
+void OptionDouble::set(const QString& s)
+{
+  value_ = s;
+
+  // Fatal on conversion error.
+  QString* endp = allow_trailing_data_? &end_: nullptr;
+  constexpr bool* dieonerror = nullptr;
+  result_ = parse_double(value_, id_, dieonerror, endp);
+}
+
+double OptionDouble::get_result(QString* end) const
+{
+  if (end != nullptr) {
+    *end = end_;
+  }
+  return result_;
+}

diff --git a/option.h b/option.h
index db4051a52f4f7b214b77f4d8c8d5ec558cf97a4a..f627dab659a9c334e96415d6d4edff58a9e5249b 100644 (file)
--- a/option.h
+++ b/option.h
@@ -19,7 +19,6 @@
 #ifndef OPTION_H_INCLUDED_
 #define OPTION_H_INCLUDED_
 
-#include <QByteArray>  // for QByteArray
 #include <QString>     // for QString, operator!=
 
 class Option /* Abstract Class */
@@ -42,73 +41,17 @@ public:
 
   /* Member Functions */
   [[nodiscard]] virtual bool has_value() const = 0;
-  virtual void reset() = 0;
   [[nodiscard]] virtual bool isEmpty() const = 0;
   [[nodiscard]] virtual const QString& get() const = 0;
+  virtual void init(const QString& id, bool allow_trailing_data, int base) {}
+  virtual void reset() = 0;
   virtual void set(const QString& s) = 0;
-  virtual void set_id(const QString& id)
-  {
-  }
 
   /* Data Members */
   // I.25: Prefer empty abstract classes as interfaces to class hierarchies
   // https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#i25-prefer-empty-abstract-classes-as-interfaces-to-class-hierarchies
 };
 
-class [[deprecated]] OptionCString : public Option
-{
-public:
-  /* Special Member Functions */
-  OptionCString() = default;
-
-  /* Traditionally a nullptr value indicated the option was not supplied.
-   * This was convenient because a char* can be implicitly converted to bool,
-   * although now we also have the equivalent member function has_value().
-   * Because QByteArray::constData() != nullptr for a null QByteArray we
-   * have to handle that case manually.
-   */
-  explicit(false) operator const char* () const
-  {
-    return value_.isNull()? nullptr : valueb_.constData();
-  }
-
-  [[nodiscard]] bool has_value() const override
-  {
-    return !value_.isNull();
-  }
-
-  void reset() override
-  {
-    value_ = QString();
-    valueb_ = QByteArray();
-  }
-
-  [[nodiscard]] bool isEmpty() const override
-  {
-    return value_.isEmpty();
-  }
-
-  [[nodiscard]] const QString& get() const override
-  {
-    return value_;
-  }
-
-  [[nodiscard]] const QByteArray& getba() const
-  {
-    return valueb_;
-  }
-
-  void set(const QString& s) override
-  {
-    value_ = s;
-    valueb_ = s.toUtf8();
-  }
-
-private:
-  QString value_;
-  QByteArray valueb_;
-};
-
 class OptionString : public Option
 {
 public:
@@ -130,11 +73,6 @@ public:
     return !value_.isNull();
   }
 
-  void reset() override
-  {
-    value_ = QString();
-  }
-
   [[nodiscard]] bool isEmpty() const override
   {
     return value_.isEmpty();
@@ -145,14 +83,19 @@ public:
     return value_;
   }
 
-  void set(const QString& s) override
+  void init(const QString& id, bool /* allow_trailing_data */, int /* base */) override
   {
-    value_ = s;
+    id_ = id;
+  }
+
+  void reset() override
+  {
+    value_ = QString();
   }
 
-  void set_id(const QString& id) override
+  void set(const QString& s) override
   {
-    id_ = id;
+    value_ = s;
   }
 
 // We use overloads instead of default parameters to enable tool visibility into different usages.
@@ -189,13 +132,6 @@ public:
     return !value_.isNull();
   }
 
-  void reset() override
-  {
-    value_ = QString();
-    result_ = 0;
-    end_ = QString();
-  }
-
   [[nodiscard]] bool isEmpty() const override
   {
     return value_.isEmpty();
@@ -206,34 +142,18 @@ public:
     return value_;
   }
 
-  void set(const QString& s) override
-  {
-    value_ = s;
-  }
-
-  void set_id(const QString& id) override
-  {
-    id_ = id;
-  }
-
-  void set_result(int result, const QString& end)
-  {
-    result_ = result;
-    end_ = end;
-  }
-
-  int get_result(QString* end = nullptr) const {
-    if (end != nullptr) {
-      *end = end_;
-    }
-    return result_;
-  }
+  void init(const QString& id, bool allow_trailing_data, int base) override;
+  void reset() override;
+  void set(const QString& s) override;
+  int get_result(QString* end = nullptr) const;
 
 private:
   QString value_;
   QString id_;
   int result_{};
   QString end_;
+  int base_{10};
+  bool allow_trailing_data_{false};
 };
 
 class OptionDouble : public Option
@@ -257,13 +177,6 @@ public:
     return !value_.isNull();
   }
 
-  void reset() override
-  {
-    value_ = QString();
-    result_ = 0.0;
-    end_ = QString();
-  }
-
   [[nodiscard]] bool isEmpty() const override
   {
     return value_.isEmpty();
@@ -274,34 +187,17 @@ public:
     return value_;
   }
 
-  void set(const QString& s) override
-  {
-    value_ = s;
-  }
-
-  void set_id(const QString& id) override
-  {
-    id_ = id;
-  }
-
-  void set_result(double result, const QString& end)
-  {
-    result_ = result;
-    end_ = end;
-  }
-
-  double get_result(QString* end = nullptr) const {
-    if (end != nullptr) {
-      *end = end_;
-    }
-    return result_;
-  }
+  void init(const QString& id, bool allow_trailing_data, int /* base */) override;
+  void reset() override;
+  void set(const QString& s) override;
+  double get_result(QString* end = nullptr) const;
 
 private:
   QString value_;
   QString id_;
   double result_{};
   QString end_;
+  bool allow_trailing_data_{false};
 };
 
 class OptionBool : public Option
@@ -324,11 +220,6 @@ public:
     return !value_.isNull();
   }
 
-  void reset() override
-  {
-    value_ = QString();
-  }
-
   [[nodiscard]] bool isEmpty() const override
   {
     return value_.isEmpty();
@@ -339,6 +230,11 @@ public:
     return value_;
   }
 
+  void reset() override
+  {
+    value_ = QString();
+  }
+
   void set(const QString& s) override
   {
     value_ = s;

diff --git a/vecs.cc b/vecs.cc
index 7e34a9f0632f7915a076d60d793ffba8d98bde5b..eb68674202d4db6008b9e24f7434c774b01d34de 100644 (file)
--- a/vecs.cc
+++ b/vecs.cc
@@ -497,7 +497,7 @@ Vecs& Vecs::Instance()
  * The possibility of detachment is also why the type of element
  * on the list must be default constructable. This is why we have
  * to supply a default for any const members of arglist_t.  Without the
- * default intializer the default constructor would be implicitly deleted.
+ * default initializer the default constructor would be implicitly deleted.
  */
 
 void Vecs::init_vec(Format* fmt)
@@ -556,13 +556,6 @@ bool Vecs::is_integer(const QString& val, const QString& id, uint32_t argtype)
   return ok;
 }
 
-int Vecs::convert_integer(const QString& val, const QString& id, QString* end, int base)
-{
-  // Fatal on conversion error
-  constexpr bool* dieonerror = nullptr;
-  return parse_integer(val, id, dieonerror, end, base);
-}
-
 bool Vecs::is_float(const QString& val, const QString& id, uint32_t argtype)
 {
   bool ok;
@@ -572,13 +565,6 @@ bool Vecs::is_float(const QString& val, const QString& id, uint32_t argtype)
   return ok;
 }
 
-double Vecs::convert_float(const QString& val, const QString& id, QString* end)
-{
-  // Fatal on conversion error
-  constexpr bool* dieonerror = nullptr;
-  return parse_double(val, id, dieonerror, end);
-}
-
 bool Vecs::is_bool(const QString& val)
 {
   return val.startsWith('y', Qt::CaseInsensitive) ||
@@ -624,7 +610,9 @@ void Vecs::assign_option(const QString& module, arglist_t& arg, const QString& v
   }
 
   arg.argval->reset();
-  arg.argval->set_id(id);
+  int base = integer_base(arg.argtype);
+  bool allow_trailing_data = trailing_data_allowed(arg.argtype);
+  arg.argval->init(id, allow_trailing_data, base);
 
   if (val.isNull()) {
     return;
@@ -632,29 +620,14 @@ void Vecs::assign_option(const QString& module, arglist_t& arg, const QString& v
 
   QString rval(val);
 
-  QString end;
-  QString* endp = trailing_data_allowed(arg.argtype)? &end: nullptr;
-
   if (auto* int_option = dynamic_cast<OptionInt*>(arg.argval); int_option != nullptr) {
-    int result;
     if (val.isEmpty()) {
       rval = '0';
-      result = 0;
-    } else {
-      // will fatal on conversion error
-      result = convert_integer(val, id, endp, integer_base(arg.argtype));
     }
-    int_option->set_result(result, end);
   } else if (auto* double_option = dynamic_cast<OptionDouble*>(arg.argval); double_option != nullptr) {
-    double result;
     if (val.isEmpty()) {
       rval = '0';
-      result = 0.0;
-    } else {
-      // will fatal on conversion error
-      result = convert_float(val, id, endp);
     }
-    double_option->set_result(result, end);
   } else if (auto* bool_option = dynamic_cast<OptionBool*>(arg.argval); bool_option != nullptr) {
     if (val.isEmpty()) {
       rval = '1';

diff --git a/vecs.h b/vecs.h
index 118d09499b6a4cf6fbfb4899c88db0f0a3f57cc6..43676137bb7e62c34e7ce1eade218b8c5304e0ae 100644 (file)
--- a/vecs.h
+++ b/vecs.h
@@ -146,9 +146,7 @@ private:
   static int integer_base(uint32_t argtype);
   static bool trailing_data_allowed(uint32_t argtype);
   static bool is_integer(const QString& val, const QString& id, uint32_t argtype);
-  static int convert_integer(const QString& val, const QString& id, QString* end, int base);
   static bool is_float(const QString& val, const QString& id, uint32_t argtype);
-  static double convert_float(const QString& val, const QString& id, QString* end);
   static bool is_bool(const QString& val);
   static QVector<style_vec_t> create_style_vec();
   QVector<vecinfo_t> sort_and_unify_vecs() const;